Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

change download-ci-llvm default from if-unchanged to true #130529

Merged
merged 3 commits into from
Sep 19, 2024

Conversation

onur-ozkan
Copy link
Member

Since #129473 and #130202, using download-ci-llvm=true is now the better default and it also fixes #130515.

@rustbot
Copy link
Collaborator

rustbot commented Sep 18, 2024

r? @Mark-Simulacrum

rustbot has assigned @Mark-Simulacrum.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels Sep 18, 2024
@rustbot
Copy link
Collaborator

rustbot commented Sep 18, 2024

This PR modifies config.example.toml.

If appropriate, please update CONFIG_CHANGE_HISTORY in src/bootstrap/src/utils/change_tracker.rs.

This PR modifies src/bootstrap/src/core/config.

If appropriate, please update CONFIG_CHANGE_HISTORY in src/bootstrap/src/utils/change_tracker.rs.

@Kobzol
Copy link
Contributor

Kobzol commented Sep 19, 2024

Why does the default behave differently than setting true explicitly?

@onur-ozkan
Copy link
Member Author

Why does the default behave differently than setting true explicitly?

The default is "if-unchanged" which fetches the LLVM submodule, but setting it to true does not.

@Kobzol
Copy link
Contributor

Kobzol commented Sep 19, 2024

I meant after this PR. I thought that when you don't set download-ci-llvm, it will behave as if you set it to true. But the code paths for "unset" and "true" look different.

@onur-ozkan
Copy link
Member Author

Hmm. Yeah, that makes sense. Should we set it true unconditionally if it's not provided from config.toml?

@Kobzol
Copy link
Contributor

Kobzol commented Sep 19, 2024

Making it behave differently would introduce essentially another mode and would be unintuitive. When it's not set, it should IMO behave exactly the same as if the user used "true".

We can just unify the logic by restructuring the pattern matching, I suppose?

@onur-ozkan onur-ozkan force-pushed the better-ci-llvm-default branch 2 times, most recently from 025352b to b11ec6b Compare September 19, 2024 08:30
@Kobzol
Copy link
Contributor

Kobzol commented Sep 19, 2024

I expressed myself badly 😆 What I meant was that the code executed if (self.channel == "dev" || self.download_rustc_commit.is_some()) holds should be the same as the code executed if the user sets true. Before, that was not the case, because the llvm::is_ci_llvm_available(self, asserts) call was missing.

That being said, just using true as the actual default value simplifies things, so if it's working, that's even better. I don't suppose that beta or stable contains local modifications to LLVM, so it should be fine, hopefully.

You can r=me once CI is green.

@onur-ozkan
Copy link
Member Author

That being said, just using true as the actual default value simplifies things, so if it's working, that's even better. I don't suppose that beta or stable contains local modifications to LLVM, so it should be fine, hopefully.

AFAIK building it from scratch is truly necessary only for distribution runners and the "dist" profile already overrides the default with false.

@rust-log-analyzer

This comment has been minimized.

Signed-off-by: onur-ozkan <work@onurozkan.dev>
Signed-off-by: onur-ozkan <work@onurozkan.dev>
Signed-off-by: onur-ozkan <work@onurozkan.dev>
@onur-ozkan
Copy link
Member Author

@bors r=Kobzol

@bors
Copy link
Contributor

bors commented Sep 19, 2024

📌 Commit 05f10f4 has been approved by Kobzol

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 19, 2024
@bors
Copy link
Contributor

bors commented Sep 19, 2024

⌛ Testing commit 05f10f4 with merge 13a5097...

@bors
Copy link
Contributor

bors commented Sep 19, 2024

☀️ Test successful - checks-actions
Approved by: Kobzol
Pushing 13a5097 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 19, 2024
@bors bors merged commit 13a5097 into rust-lang:master Sep 19, 2024
7 checks passed
@rustbot rustbot added this to the 1.83.0 milestone Sep 19, 2024
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (13a5097): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

This benchmark run did not return any relevant results for this metric.

Cycles

Results (secondary -5.1%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-5.1% [-5.1%, -5.1%] 1
All ❌✅ (primary) - - 0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 769.526s -> 769.055s (-0.06%)
Artifact size: 341.30 MiB -> 341.32 MiB (0.01%)

@onur-ozkan onur-ozkan deleted the better-ci-llvm-default branch September 20, 2024 07:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

x setup tries to check out src/llvm-project
8 participants